[clang][AArch64] Avoid a crash when a non-reserved register is used #117419
Conversation
Fixes llvm#76426 The previous patch for this issue, llvm#94271, generated an error message if a register and a global variable did not have the same size. This patch checks if the register is actually reserved.
igorkudrin
added
backend:AArch64
clang:frontend
igorkudrin
requested review from
echristo,
DanielKristofKiss,
kawashima-fj,
ahatanak and
arakawamasahiro-fj
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Igor Kudrin (igorkudrin) ChangesFixes #76426 The previous patch for this issue, #94271, generated an error message if a register and a global variable did not have the same size. This patch checks if the register is actually reserved. Full diff: https://github.com/llvm/llvm-project/pull/117419.diff 2 Files Affected:
diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index eb8a3ada034482..7500c7bb045e54 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -232,13 +232,23 @@ bool AArch64TargetInfo::validateTarget(DiagnosticsEngine &Diags) const {
bool AArch64TargetInfo::validateGlobalRegisterVariable(
StringRef RegName, unsigned RegSize, bool &HasSizeMismatch) const {
- if ((RegName == "sp") || RegName.starts_with("x")) {
- HasSizeMismatch = RegSize != 64;
- return true;
- } else if (RegName.starts_with("w")) {
+ if (RegName.starts_with("w")) {
HasSizeMismatch = RegSize != 32;
return true;
}
+ if (RegName == "sp") {
+ HasSizeMismatch = RegSize != 64;
+ return true;
+ }
+ if (RegName.starts_with("x")) {
+ HasSizeMismatch = RegSize != 64;
+ // Check if the register is reserved. See also
+ // AArch64TargetLowering::getRegisterByName().
+ return RegName == "x0" ||
+ (RegName == "x18" &&
+ llvm::AArch64::isX18ReservedByDefault(getTriple())) ||
+ getTargetOpts().FeatureMap.lookup(("reserve-" + RegName).str());
+ }
return false;
}
diff --git a/clang/test/Sema/aarch64-fixed-global-register.c b/clang/test/Sema/aarch64-fixed-global-register.c
index 9b4a422d8c1b2c..2b7d39dbcdd6f8 100644
--- a/clang/test/Sema/aarch64-fixed-global-register.c
+++ b/clang/test/Sema/aarch64-fixed-global-register.c
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -triple aarch64-unknown-none-gnu %s -verify -fsyntax-only
+// RUN: %clang_cc1 -triple aarch64-unknown-none-gnu %s -target-feature +reserve-x4 -target-feature +reserve-x15 -verify -fsyntax-only
register char i1 __asm__ ("x15"); // expected-error {{size of register 'x15' does not match variable size}}
register long long l2 __asm__ ("w14"); // expected-error {{size of register 'w14' does not match variable size}}
+register long x3 __asm__ ("x3"); // expected-error {{register 'x3' unsuitable for global register variables on this target}}
+register long x4 __asm__ ("x4");
|
|
Please check if this fixes #109778 too. Seems very similar. |
llvmbot
added
the
clang:driver
|
Thanks, I'll leave it open then, this PR is a welcome improvement regardless. |
clang/lib/Basic/Targets/AArch64.cpp
Outdated
| return RegName == "x0" || | ||
| (RegName == "x18" && | ||
| llvm::AArch64::isX18ReservedByDefault(getTriple())) || | ||
| getTargetOpts().FeatureMap.lookup(("reserve-" + RegName).str()); |
There was a problem hiding this comment.
To avoid the fatal error: error in backend: Invalid register name "x??" error in #109778, this check is sufficient.
I think the original purpose of rejecting a global register variable associated with a non-reserved register is to avoid unintended register use. w?? registers are lower half bits of the corresponding x?? registers. So when x?? register is not reserved, should we reject a global register variable associated with the corresponding w???
For this purpose, maybe the backend should also check w?? registers. Actually, old backend code checked w?? regsters.
There was a problem hiding this comment.
Thanks! Since this patch is focused on fixing the crash, it doesn't seem appropriate to make AArch64TargetLowering::getRegisterByName() more restrictive here. However, I added checks for w?? registers in AArch64TargetInfo::validateGlobalRegisterVariable() and corresponding tests.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/13353 Here is the relevant piece of the build log for the reference |
…s used (#117419) Relanding the patch with a fix for a test failure on build bots that do not build LLVM for AArch64. Fixes #76426, #109778 (for AArch64) The previous patch for this issue, #94271, generated an error message if a register and a global variable did not have the same size. This patch checks if the register is reserved.
…lvm#117419) Fixes llvm#76426, llvm#109778 (for AArch64) The previous patch for this issue, llvm#94271, generated an error message if a register and a global variable did not have the same size. This patch checks if the register is reserved.
…s used (llvm#117419)" This reverts commit 8fc6fca.
…s used (llvm#117419) Relanding the patch with a fix for a test failure on build bots that do not build LLVM for AArch64. Fixes llvm#76426, llvm#109778 (for AArch64) The previous patch for this issue, llvm#94271, generated an error message if a register and a global variable did not have the same size. This patch checks if the register is reserved.
Fixes #76426, #109778 (for AArch64)
The previous patch for this issue, #94271, generated an error message if a register and a global variable did not have the same size. This patch checks if the register is reserved.